Flicker refactor #161
Merged
Flicker refactor #161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… - still have to add to config
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 81.90% 82.60% +0.69%
==========================================
Files 114 117 +3
Lines 12289 12852 +563
==========================================
+ Hits 10065 10616 +551
- Misses 2224 2236 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Major refactor of flicker to merge a good portion of the setbacks functionality into the flicker routines. In particular, the major goals for this refactor were as follows:
FLICKER_ARRAY_LEN, such that it no longer errors out for low flicker thresholds or large turbines.This list is not comprehensive, but it does provide the most important updates. All of these issues were identified during the last set of bespoke runs, and were needed to be fixed before any new runs were to be done.
The core technical component of this refactor was to move the logic for running and merging exclusions out of the base setbacks class and into a general-utility framework. The new framework class is now defined in
reVX.utilties.exclusions.AbstractBaseExclusionsMerger, with an explicit interface defined inreVX.utilties.exclusions.AbstractExclusionCalculatorInterface. Both the setbacks and the flicker routines implement this interface, allowing both routines to share the logic of calculating local and generic exclusions and merging those together. An added bonus is that the CLI and therefore the configs share the inputs required to perform local and generic merging, so analysist that are familiar with setbacks configs designed with that functionality should be able to apply the same inputs to the flicker configs.One thing I debated for a long time was a re-organization of the folder structure. In particular, it made a lot of sense to me to create a new "exclusions" module that would hold both the flicker and the setbacks folders. Ultimately I did not end up implementing this change since I felt it may be a bit too disruptive, but I am still very much on the fence about this decision and would happily implement it if it makes sense to others.
All new features should have tests, but I will be keeping a close eye on the coverage report to see if I missed anything major.